Adds DeprecatedHelperVisitor #659
Conversation
…ing in the 3rd argument of the DeprecationHelper, making it ignore deprecation messages in there.
bbrala
force-pushed
the
feature/deprecation-helper-fixer
branch
from
ba47283 to
47de7e3
Compare
| @@ -0,0 +1,26 @@ | |||
| <?php | |||
|
|
|||
| namespace GroupLegacy; | |||
There was a problem hiding this comment.
This should be a more sane namespace i guess
There was a problem hiding this comment.
It's good enough for the test data :)
| { | ||
|
|
||
| if ($node instanceof Node\Expr\StaticCall) { | ||
| if ($node->class instanceof Node\Name\FullyQualified && $node->class->isFullyQualified() && $node->class->toString() === 'Drupal\Component\Utility\DeprecationHelper') { |
There was a problem hiding this comment.
It's always an instance of FullyQualified even if imported, right? Pretty sure but double checking.
There was a problem hiding this comment.
yeah i removed this one after your review
| $this->deprecatedCall = null; | ||
| return NodeTraverser::DONT_TRAVERSE_CURRENT_AND_CHILDREN; | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
Is there a chance our property won't get unset for some reason?
There was a problem hiding this comment.
Calling deprecation helper without arguments? But that is impossible.
Perhaps if while moving into the function something changes the node, so the argument is no longer on the same line and therefor not equal? Not sure how, and if that is possible.
But the effect would basically be if would check the array a few times extra, which should be quite cheap.
There might be a way to add some more checks when in the argument. Or do something like keeping a stack of the last static call and check if that call is nothing weird.
if ($node instanceof Node\Expr\StaticCall) {
$this->staticCall = $node; // this and check this?
if ($node->class instanceof Node\Name\FullyQualified && $node->class->toString() === 'Drupal\Component\Utility\DeprecationHelper') {
$this->deprecatedCall = $node->getArgs()[2];
return null;
}
}
But that seems like overkill.
| } | ||
|
|
||
| if ($this->deprecatedCall !== null && $node instanceof Node\Arg && $this->isSavedArgument($node)) { | ||
| $this->deprecatedCall = null; |
There was a problem hiding this comment.
The null check is technically duplicated in the isSavedArgument method
There was a problem hiding this comment.
I was making phpstan happy, the property can be null.
| private function isSavedArgument(Node\Arg $node): bool | ||
| { | ||
| if ($this->deprecatedCall !== null && $node->getAttributes() === $this->deprecatedCall->getAttributes()) { | ||
| return true; |
There was a problem hiding this comment.
Okay so when we detect a static call on the methods, we fetch and stash the callable for the deprecated case. Then when then node parser reaches the argument we check and unset.
| if ($this->deprecatedCall !== null && $node instanceof Node\Arg && $this->isSavedArgument($node)) { | ||
| $this->deprecatedCall = null; | ||
| return NodeTraverser::DONT_TRAVERSE_CURRENT_AND_CHILDREN; | ||
| } |
There was a problem hiding this comment.
This is the magic, yeah? PHPStan doesn't analyze anything in the tree? Seems problematic and instead we should try adding information if possible. Probably not, though.
There was a problem hiding this comment.
Yeah, it stops parsing as discussed on slack.
|
Hi @mglaman , any chance you can revisit this? I understand the drawbacks, but we need some solution to skip code inside the deprecations helper before we start posting patches. |
|
:/ sorry it's been in my TODOs
I'll get to this shortly. I'm thinking we can enable it behind a feature flag. The project update bot can opt in. I'm still concerned that it just halts analysis. |
|
This has been superceeded by #714, yay for phpstan developer @ondrejmirtes (and you @mglaman ;)) |
Adds DeprecatedHelperVisitor which removes PHPStan parsing of everything in the 3rd argument of the DeprecationHelper, making it ignore deprecation messages in there.